Skip to content

feat(webvh): Added did:webvh resolver #2238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

brianorwhatever
Copy link

@genaris @TimoGlastra as discussed I have pulled the crypto out of didwebvh-ts while also removing all of the dependencies. Please take a look at this new PR

@brianorwhatever brianorwhatever requested a review from a team as a code owner March 24, 2025 22:48
Copy link

changeset-bot bot commented Mar 24, 2025

🦋 Changeset detected

Latest commit: 884d3af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@credo-ts/webvh Minor
@credo-ts/action-menu Minor
@credo-ts/anoncreds Minor
@credo-ts/askar Minor
@credo-ts/cheqd Minor
@credo-ts/core Minor
@credo-ts/didcomm Minor
@credo-ts/drpc Minor
@credo-ts/indy-sdk-to-askar-migration Minor
@credo-ts/indy-vdr Minor
@credo-ts/node Minor
@credo-ts/openid4vc Minor
@credo-ts/question-answer Minor
@credo-ts/react-native Minor
@credo-ts/redis-cache Minor
@credo-ts/tenants Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new approach is awesome, thanks @brianorwhatever. I left some comments. It seems some parts are todo, do you want to address these as part of this PR, or in another one?

},
}
}
const privateKey = Buffer.from(MultiBaseEncoder.decode(privateKeyMultibase).data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to pass an existing public key, but not the private key? We usually create a key first, then use that to create a diddoc

(This is fine to add later, just trying to understand how it works)

throw new Error('Not implemented')
}

public async verify(signature: Uint8Array, message: Uint8Array, publicKey: Uint8Array): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably good if the interface from did web vh passes the keytype as well, so it's ready to support additional key types as well

export class DIDWebvhCrypto extends AbstractCrypto {
private agentContext?: AgentContext

public constructor(agentContext?: AgentContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make agent context required

@brianorwhatever
Copy link
Author

@TimoGlastra I forgot to mention I was focussing solely on resolver in this PR. I added some of the base stuff for the registrar, which will be coming soon. I'm happy to either remove that stuff or fix it up later. Higher priority is resolving DID documents and resources.

@TimoGlastra
Copy link
Contributor

Ah that makes sense @brianorwhatever. Let's focus this pr on the resolving in that case and we can add the registration parts next 👍

@TimoGlastra
Copy link
Contributor

Curious is the implementation backwards compatible with older versions?

The swiss eid public beta that was announced yesterday uses version 0.3. I'm trying to understand if we could use this resolver / didwebvh-ts for integrating with this?

@brianorwhatever
Copy link
Author

OK, great I will remove the stuff about the registrar. Unfortunately, I don't think that this is 0.3 compatible I can compare the differences and see what would be required to make it work.

@brianorwhatever
Copy link
Author

Hey @TimoGlastra, quick question about verifying Data Integrity proofs in a custom setup.

We're building an AnonCredsRegistry for did:webvh (WebVhAnonCredsRegistry). The objects we fetch (schemas, cred defs, etc.) are structured as simple JSON-LD "AttestedResources" which basically wrap the content (the actual AnonCreds object) and include a standard DI proof (e.g., eddsa-jcs-2022).

Inside our registry logic (in a helper _resolveAndValidateAttestedResource), we're already handling:

  • Resolving the did:webvh:.../resource/hash URI.
  • Parsing the JSON-LD.
  • Checking the type includes AttestedResource.
  • Verifying a multihash of the canonicalized content matches the hash in the URI.
  • Checking the issuerId in the content matches the DID from the URI.

The last piece is actually verifying the proof signature itself. We looked into DataIntegrityService and W3cCredentialService, but they didn't seem designed for verifying proofs on arbitrary signed objects like this (it's not strictly a VC/VP).

We also considered using the signature suites directly (like Ed25519Signature2020), getting them from the SignatureSuiteRegistry, and calling their verifyProof method. This looks possible but feels like we might be duplicating internal logic, especially around preparing the data correctly for the specific suite and ensuring the documentLoader (which uses our WebvhDidResolver) is passed through correctly to resolve the verificationMethod.

Is there a recommended/simpler way in Credo to verify a DI proof on a custom JSON-LD object like this, making sure key resolution via the verificationMethod works as expected? Or is using the signature suite directly the intended path here?

Currently have it stubbed out, but want to make sure we implement the crypto verification correctly using Credo's infrastructure.

Thanks!

@brianorwhatever
Copy link
Author

@TimoGlastra @genaris this is ready for a review. I have updated the didwebvh-ts package to the new version which supports version 1.0 of the spec and addressed any comments in this thread. Thanks!

@brianorwhatever
Copy link
Author

The integration tests probably don't need to be checked in they are testing real resource resolution and those files will likely disappear one day

@@ -0,0 +1,39 @@
{
"name": "@credo-ts/webvh",
"version": "0.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be synced with the other packages

Suggested change
"version": "0.4.0",
"version": "0.5.13",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a multihash implementation already: https://github.com/openwallet-foundation/credo-ts/blob/879ed2c9178684d14e6850cbcdb02d6832e0d730/packages/core/src/utils/MultiHashEncoder.ts.

If there's any features missing in our multi hash implementation, it would be better to extend the existing functionaility, this way we prevent duplication of logic

@Transform(({ value }) => {
// Determine which class to use based on the content
if (value && 'attrNames' in value) {
return Object.assign(new WebVhSchemaContent(), value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this stil trigger the validation i you're doing it like this? Usually i use the JsonTransformer, but if this works, it's ok 👍

Comment on lines 10 to 11
public readonly allowsCaching = false
public readonly allowsLocalDidRecord = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason these are both false? For the did:web one they are both true, I'd assume the same values here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was temporary and I forgot to update. true seems fine to me will set them to that.

agentContext.config.logger.debug(`Fetching resource from: ${httpsUrl}`)

// Fetch the resource using native fetch
const response = await fetch(httpsUrl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use fetch from the agent config. agentContext.config.agentDependencies.fetch

Comment on lines +9 to +15
verificationMethod: {
id: 'did:webvh:123',
controller: 'did:webvh:123',
type: 'Ed25519VerificationKey2020',
publicKeyMultibase: '123',
secretKeyMultibase: '123',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we set it to these sample values? Should this be optional in the AbstractCrypto?

Comment on lines +24 to +30
private async isKmsAvailable(): Promise<boolean> {
try {
return !!(this.agentContext.dependencyManager.resolve(Kms.KeyManagementApi))
} catch {
return false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kms will always be available, so i think this check is redundant

}
}

private async verifyWithLegacyMethod(signature: Uint8Array, message: Uint8Array, publicKey: Uint8Array): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed, if you want this to work with 0.5.x as well, you can make another PR to the 0.5.x branch based on the old KMS

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to an example of the old KMS? I couldn't find any. This was specifically not working in BC Wallet which is on 0.5.13.

Is main branch for version 0.6.0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the main branch is for 0.6.0.

If we want this to be available in both i think the best method is to:

  • remove all legacy KMs stuff from this PR
  • merge into main
  • create a PR to 0.5.x branch, with changes for the legacy KMS

But when targeting the main branch, we should only use the new KMS API

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, that makes sense. I'm in the process of removing the legacy stuff and I think I have an idea of how to get 0.5 working. Just created a PR for didwebvh-ts that removes the requirement of passing a VM into the constructor so once that lands I'll have this PR in great shape

Comment on lines 46 to 52
agentContext = getAgentContext({ agentConfig })

// Register the mocked resolver instance
agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver())

// Mock the dependency manager resolve method
const originalResolve = agentContext.dependencyManager.resolve.bind(agentContext.dependencyManager)
agentContext.dependencyManager.resolve = jest.fn().mockImplementation((token) => {
const tokenString = token?.name || token?.toString?.() || String(token)

if (tokenString.includes('DidsApi')) {
return mockDidsApi
}
if (tokenString.includes('KeyManagementApi')) {
return mockKeyManagementApi
}
if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) {
return { resolveResource: mockResolveResource }
}
return originalResolve(token)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like this would also work:

Suggested change
agentContext = getAgentContext({ agentConfig })
// Register the mocked resolver instance
agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver())
// Mock the dependency manager resolve method
const originalResolve = agentContext.dependencyManager.resolve.bind(agentContext.dependencyManager)
agentContext.dependencyManager.resolve = jest.fn().mockImplementation((token) => {
const tokenString = token?.name || token?.toString?.() || String(token)
if (tokenString.includes('DidsApi')) {
return mockDidsApi
}
if (tokenString.includes('KeyManagementApi')) {
return mockKeyManagementApi
}
if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) {
return { resolveResource: mockResolveResource }
}
return originalResolve(token)
})
agentContext = getAgentContext({
agentConfig,
registerInstances: [
[DidsApi, mockDidsApi],
[KeyManagementApi, mockKeyManagementApi],
[WebvhDidResolver, { resolveResource: mockResolveResource }]
]
})

Not sure about this line:

    // Register the mocked resolver instance
    agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver())

As afterwards you overwrite it here:

      if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) {
        return { resolveResource: mockResolveResource }
      }

You can update the entry in registerInstances for WebvhDidResolver with the value you like.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worked great. thanks!

@brianorwhatever
Copy link
Author

Thank you @TimoGlastra I believe I addressed all of your comments other than the legacy KMS stuff. Need a bit more help getting that plugged in properly

Signed-off-by: Brian Richter <brian@aviary.tech>
- Removed WebvhDidRegistrar class and its related exports from the dids module.
- Updated index.ts and WebvhModule.ts to reflect the removal of the registrar.
- Adjusted DIDWebvhCrypto class to ensure agentContext is always defined.
- Cleaned up unused functions and imports in didWebvhUtil.ts.

Signed-off-by: Brian Richter <brian@aviary.tech>
…ial definition support

- Added WebVhAnonCredsRegistry class to handle anon credentials with did:webvh identifiers.
- Implemented methods for retrieving schemas, credential definitions, and revocation registry definitions.
- Introduced validation and error handling for resource resolution and proof verification.
- Created utility functions for multihash encoding and base58 conversion.
- Added tests for WebVhAnonCredsRegistry and resource transformation.
- Updated package.json to include new dependencies for class-transformer and class-validator.

Signed-off-by: Brian Richter <brian@aviary.tech>
- Updated valid DID and related references in tests to reflect the proper DID format.
- Ensured consistency across the test suite by replacing old example DIDs with the updated structure.

Signed-off-by: Brian Richter <brian@aviary.tech>
…itialization

- Deleted WebvhModuleConfig class and its related exports to streamline the module configuration.
- Updated WebvhModule to remove dependency on WebvhModuleConfig, simplifying its constructor.
- Adjusted test setup to reflect the removal of configuration options.

Signed-off-by: Brian Richter <brian@aviary.tech>
… method signatures

- Removed unused imports and streamlined type definitions for resource resolution results.
- Updated method signatures to reflect changes in type definitions, enhancing clarity.
- Consolidated register methods to eliminate unnecessary parameters, maintaining consistency in error handling.

Signed-off-by: Brian Richter <brian@aviary.tech>
…atibility

- Updated 'didwebvh-ts' version to ^2.2.0 in package.json.

Signed-off-by: Brian Richter <brian@aviary.tech>
- Updated TypeScript version from 4.9.5 to 5.5.4 in package.json and pnpm-lock.yaml for improved compatibility.
- Removed the obsolete tsconfig.test.json file to streamline project structure.
- Simplified tsconfig.json by removing unnecessary path mappings.
- Adjusted test files to reflect changes in type definitions and imports.

Signed-off-by: Brian Richter <brian@aviary.tech>
…nd resource resolution improvements

- Implemented a new `verifyProof` method for validating DataIntegrityProofs using eddsa-jcs-2022.
- Enhanced error handling in `_resolveAndValidateAttestedResource` for better clarity on resolution failures.
- Updated the `WebvhDidResolver` to fetch resources from the specified URLs and handle various content types.
- Added comprehensive tests for the new proof verification logic and resource resolution, ensuring robustness and reliability.

Signed-off-by: Brian Richter <brian@aviary.tech>
…with improved crypto handling and resource resolution

- Introduced `DIDWebvhCrypto` for streamlined signature verification using JWK and KMS.
- Updated `WebVhAnonCredsRegistry` to utilize the new crypto class for signature verification.
- Enhanced `WebvhDidResolver` with a new `getBaseUrl` method for better DID URL parsing.
- Removed obsolete test cases and added integration tests for `WebvhDidResolver` to ensure robust resource resolution.

Signed-off-by: Brian Richter <brian@aviary.tech>
…methods

- Added '@stablelib/ed25519' dependency for enhanced Ed25519 signature verification.
- Updated 'didwebvh-ts' version to ^2.3.2 for improved functionality.
- Refactored signature verification in DIDWebvhCrypto to utilize KMS or fallback to legacy methods as needed.

Signed-off-by: Brian Richter <brian@aviary.tech>
- Added MultiBaseEncoder and MultiHashEncoder to improve encoding and decoding processes.
- Updated WebVhAnonCredsRegistry to utilize new encoding methods for hash generation.
- Refactored utility functions to remove deprecated base58 and multihash implementations.
- Enhanced error logging in WebVhAnonCredsRegistry for better clarity on issues.
- Updated tests to reflect changes in resource handling and encoding logic.

Signed-off-by: Brian Richter <brian@aviary.tech>
- Added export for WebVhAnonCredsRegistry in index.ts to enhance module accessibility.

Signed-off-by: Brian Richter <brian@aviary.tech>
"dependencies": {
"@credo-ts/anoncreds": "workspace:*",
"@credo-ts/core": "workspace:*",
"@multiformats/base-x": "^4.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed now?

"@credo-ts/anoncreds": "workspace:*",
"@credo-ts/core": "workspace:*",
"@multiformats/base-x": "^4.0.1",
"@stablelib/ed25519": "^2.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also

For the legacy KMS, we also supported Ed25519 ,so it should not be needed to use this (and we prefer doing everything through the pluggable crypto interface)

Comment on lines +195 to +205
if (error instanceof CredoError) {
if (error.message.includes('parse resource data')) errorType = 'invalidJson'
else if (error.message.includes('resolve') || error.message.includes('missing data')) errorType = 'notFound'
else if (error.message.includes('attested')) errorType = 'invalid'
else if (error.message.includes('proof')) errorType = 'invalid'
else if (error.message.includes('hash mismatch')) errorType = 'invalid'
else if (error.message.includes('not a valid schema')) errorType = 'invalid'
else if (error.message.includes('Invalid schema ID')) errorType = 'invalidDid'
else if (error.message.includes('Issuer ID mismatch')) errorType = 'invalid'
else if (error.message.includes('missing a valid issuerId')) errorType = 'invalid'
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these error messsages constructed? We don't use error messages anywhere to handle errors as it's very brittle. If you want to catch these cases, best i think is to create and throw specific errors that you can check with instanceof

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants